LCORE-3027: Debug skills E2E#2138
Conversation
WalkthroughThe response completion step now prints ChangesResponse Debugging
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/features/steps/llm_query_response.py`:
- Around line 94-96: Update the response handling around
_parse_streaming_response so the safe, redacted diagnostic is emitted
immediately after parsing and before context.response.raise_for_status().
Preserve the existing status validation and ensure failure responses still print
the parsed payload for debugging.
- Line 96: Replace the raw context.response_data print in the response-handling
step with the project logger initialized as logger = get_logger(__name__). Emit
only a bounded, redacted diagnostic that excludes response text, conversation
IDs, tool calls/results, and stream errors; do not log the full payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e391f36d-e32b-4cde-ae8b-49bc477af444
📒 Files selected for processing (1)
tests/e2e/features/steps/llm_query_response.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: list_outdated_dependencies
- GitHub Check: Pyright
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: spectral
- GitHub Check: mypy
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-7-on-pull-request
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
tests/e2e/features/steps/llm_query_response.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules and checkpyproject.tomlfor existing dependencies and versions before adding dependencies.
All Python modules must begin with descriptive docstrings; package__init__.pyfiles must contain brief package descriptions.
Uselogger = get_logger(__name__)fromlog.pyfor module logging.
UseFinal[type]for constants and checkconstants.pyfor shared constants before defining new ones.
Define type aliases at module level for clarity.
All functions must have Google-style docstrings and complete parameter and return type annotations.
Usetyping_extensions.Selffor model validators, modern union syntax such asstr | int, andOptional[Type]for optional values.
Use descriptive, action-oriented snake_case names for functions, such asget_,validate_, andcheck_.
Do not modify parameter objects in place when producing results; return a new data structure instead.
Useasync deffor I/O operations and external API calls.
HandleAPIConnectionErrorfrom Llama Stack integrations.
Use standard logger levels appropriately:debugfor diagnostics,infofor normal execution,warningfor unexpected or potentially problematic conditions, anderrorfor serious failures.
All classes must have descriptive Google-style docstrings, complete attribute type annotations, and specific types instead ofAny.
Use PascalCase for classes with descriptive suffixes, includingConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
UseABCand@abstractmethodfor abstract interfaces.
Follow Google Python docstring conventions and includeParameters,Returns,Raises, andAttributessections when applicable.
Files:
tests/e2e/features/steps/llm_query_response.py
tests/e2e/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Behave with Gherkin for end-to-end tests; place step definitions under
tests/e2e/features/steps/.
Files:
tests/e2e/features/steps/llm_query_response.py
**/*.{py,toml,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
Run and satisfy the project's configured quality checks, including Black, Pylint, Pyright, Ruff, pydocstyle, type checks, and Bandit security scanning.
Files:
tests/e2e/features/steps/llm_query_response.py
🧠 Learnings (3)
📚 Learning: 2026-04-07T09:20:26.590Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1467
File: tests/e2e/features/steps/common.py:36-49
Timestamp: 2026-04-07T09:20:26.590Z
Learning: For Behave-based Python tests, rely on Behave’s Context layered stack for attribute lifecycle: Behave pushes a new Context layer when entering feature scope (before_feature) and again for scenario scope (before_scenario). Attributes assigned inside given/when/then steps live on the current scenario layer and are automatically removed when the scenario ends. As a result, step-set attributes should not be expected to persist across scenarios or features, and manual cleanup in after_scenario/after_feature is generally unnecessary for attributes set in step functions. Only perform manual cleanup for attributes that you set explicitly in before_feature/before_scenario, since those live on the respective feature/scenario layers.
Applied to files:
tests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-04-13T13:39:54.963Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:206-211
Timestamp: 2026-04-13T13:39:54.963Z
Learning: In lightspeed-stack E2E tests under tests/e2e/features, it is intentional to set context.feature_config inside Background/step functions (scenario-scoped Behave layer). The environment.py after_scenario restore logic should only restore configuration when context.scenario_lightspeed_override_active is True; this flag is set by configure_service only when a real config switch occurs (so restore does not run for scenarios without a switch). Additionally, steps/common.py’s module-level _active_lightspeed_stack_config_basename is used to prevent re-applying the same config across subsequent scenarios, ensuring scenario_lightspeed_override_active stays False after the first apply. Therefore, reviewers should not “fix” this flow as if feature_config were incorrectly scoped or if after_scenario restoration is missing—config switching and restoration are meant to happen exactly once per actual switch, not redundantly per scenario.
Applied to files:
tests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/e2e/features/steps/llm_query_response.py
| context.response_data = _parse_streaming_response(context.response.text) | ||
| context.response.raise_for_status() | ||
| print(f"Response data: {context.response_data}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Emit the diagnostic before raise_for_status().
When the response has a non-2xx status, Line 95 raises before Line 96 executes, so the payload is unavailable for precisely those failures the debug output should help diagnose. Move the safe, redacted diagnostic immediately after parsing or include it in error handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/features/steps/llm_query_response.py` around lines 94 - 96, Update
the response handling around _parse_streaming_response so the safe, redacted
diagnostic is emitted immediately after parsing and before
context.response.raise_for_status(). Preserve the existing status validation and
ensure failure responses still print the parsed payload for debugging.
| """Wait for the response to be complete.""" | ||
| context.response_data = _parse_streaming_response(context.response.text) | ||
| context.response.raise_for_status() | ||
| print(f"Response data: {context.response_data}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not print the full response payload to CI logs.
context.response_data includes response text, conversation IDs, tool calls/results, and stream errors. Raw printing can expose sensitive data to anyone with CI-log access and produce unbounded output. Use the project logger with a bounded/redacted diagnostic instead.
As per coding guidelines, module logging must use logger = get_logger(__name__), and sensitive data must not be leaked in logs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/features/steps/llm_query_response.py` at line 96, Replace the raw
context.response_data print in the response-handling step with the project
logger initialized as logger = get_logger(__name__). Emit only a bounded,
redacted diagnostic that excludes response text, conversation IDs, tool
calls/results, and stream errors; do not log the full payload.
Source: Coding guidelines
1 participant
Description
Add debug print statement to the
wait_for_complete_responseE2E step to log the parsed streaming response data before assertions. This aids in diagnosing intermittent skills E2E test failures by making the actual response payload visible in CI output.Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Verified via CI pipeline. The added print statement outputs the parsed streaming response data during E2E test runs, providing visibility into the response payload when skills feature tests execute.
Summary by CodeRabbit